Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Jun 7, 2025

Since changing the backend to fold x >= 1 / x < 1 -> x > 0 / x <= 0 and x <= -1 / x > -1 -> x > 0 / x <= 0, this should be reflected in the cost.

@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2025

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-backend-aarch64

Author: AZero13 (AZero13)

Changes

Since changing the backend to fold x >= 1 / x < 1 -> x > 0 / x <= 0 and x <= -1 / x > -1 -> x > 0 / x <= 0, this should be reflected in the cost.


Full diff: https://github.com/llvm/llvm-project/pull/143286.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp (+15-6)
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
index 68aec80f07e1d..72b60eaa45b78 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -4346,14 +4346,23 @@ InstructionCost AArch64TTIImpl::getCmpSelInstrCost(
   }
 
   // Treat the icmp in icmp(and, 0) as free, as we can make use of ands.
-  // FIXME: This can apply to more conditions and add/sub if it can be shown to
-  // be profitable.
   if (ValTy->isIntegerTy() && ISD == ISD::SETCC && I &&
-      ICmpInst::isEquality(VecPred) &&
+      !CmpInst::isUnsigned(VecPred) &&
       TLI->isTypeLegal(TLI->getValueType(DL, ValTy)) &&
-      match(I->getOperand(1), m_Zero()) &&
-      match(I->getOperand(0), m_And(m_Value(), m_Value())))
-    return 0;
+      match(I->getOperand(0), m_And(m_Value(), m_Value()))) {
+    if (match(I->getOperand(1), m_Zero()))
+      return 0;
+
+    // x >= 1 / x < 1 -> x > 0 / x <= 0
+    if (match(I->getOperand(1), m_One()) &&
+        (VecPred == CmpInst::ICMP_SLT || VecPred == CmpInst::ICMP_SGE))
+      return 0;
+
+    // x <= -1 / x > -1 -> x > 0 / x <= 0
+    if (match(I->getOperand(1), m_AllOnes()) &&
+        (VecPred == CmpInst::ICMP_SLE || VecPred == CmpInst::ICMP_SGT))
+      return 0;
+  }
 
   // The base case handles scalable vectors fine for now, since it treats the
   // cost as 1 * legalization cost.

@AZero13 AZero13 force-pushed the signed-unsigned branch from 64fe76a to e89c054 Compare June 7, 2025 21:42
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jun 7, 2025
@AZero13
Copy link
Contributor Author

AZero13 commented Jun 13, 2025

@davemgreen Thoughts?

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi - I think this sounds OK, some of the cases will be canonicalized by the mid-end to compare against 0. Out of interest do you have some motivating example you are trying to optimize, or is this a general improvement as you were looking at the backend patterns?

Can you add some extra tests for each of the cases?

@AZero13
Copy link
Contributor Author

AZero13 commented Jun 16, 2025

The cost model needs to be accurate as possible

@AZero13 AZero13 requested a review from SamTebbs33 June 16, 2025 13:53
AZero13 added 2 commits June 16, 2025 10:27
Since changing the backend to fold x >= 1 / x < 1 -> x > 0 / x <= 0 and x <= -1 / x > -1 -> x > 0 / x <= 0, this should be reflected in the cost.
Copy link
Collaborator

@SamTebbs33 SamTebbs33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks. Do let @davemgreen have another look though.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, LGTM. Are you happy for this to be merged?

@AZero13
Copy link
Contributor Author

AZero13 commented Jun 17, 2025

Sure, LGTM. Are you happy for this to be merged?

Yes

@davemgreen davemgreen merged commit dc72b91 into llvm:main Jun 17, 2025
7 checks passed
@AZero13 AZero13 deleted the signed-unsigned branch June 17, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:AArch64 llvm:analysis Includes value tracking, cost tables and constant folding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants